Enabling biometry without password during sync #17960
Conversation
clauxx
requested review from
ilmotta,
cammellos,
Samyoul,
flexsurfer and
jo-mut
Jenkins BuildsClick to see older builds (60)
|
| [key-uid callback] | ||
| (keychain/get-credentials key-uid | ||
| #(if % | ||
| (callback (security/mask-data (oops/oget % "password"))) |
There was a problem hiding this comment.
threading maybe makes it a bit more readable:
(-> (oops/get % password)
(security/mask-data)
(callback))
or
(as-> % $
(oops/oget $ "password")
(security/mask-data $)
(callback $))
also, you can have the if inside, since you are calling callback regardless:
(callback (when %
(-> (oops/get...))
maybe there's also better ways, but it's all fine to be honest
There was a problem hiding this comment.
went with the last option
| [key-uid callback] | ||
| (keychain/get-credentials | ||
| (password-migration-key-name key-uid) | ||
| #(callback (boolean %)))) |
There was a problem hiding this comment.
(comp callback boolean) is equivalent if you prefer
| (.then #(save-user-password! key-uid %)) | ||
| (.then #(save-password-migration! key-uid)) | ||
| (.then #(callback)) | ||
| (.catch #(log/error "Failed to migrate the keychain password for " key-uid |
There was a problem hiding this comment.
@ilmotta suggested some structured logging format, maybe there's a case for it to be used here
There was a problem hiding this comment.
Right @cammellos, log/error can take pretty much anything, and the lib allows us to perform transformations with middlewares. The convention we try to follow in the repo is to pass a first argument string with a concise humanized message, and then the second argument a map giving enough context for the future dev to make sense of the error.
So here it could be:
(.catch (fn [error]
(log/error "Failed to migrate keychain password"
{:key-uid key-uid
:event :keychain/password-hash-migration
:error error})))The original code concatenating values won't be as clear. It will output something like Failed to migrate the keychain password for foo, but what is foo? Using a data structure the developer will know exactly what it is.
There was a problem hiding this comment.
Thanks, had no idea we had a convention for this! Fixed it for the keychain.events namespace.
| @@ -151,12 +162,16 @@ | |||
|
|
|||
| (rf/defn get-auth-method-success | |||
| {:events [:profile.login/get-auth-method-success]} | |||
| [{:keys [db]} auth-method] | |||
| [{:keys [db] :as cofx} auth-method key-uid] | |||
There was a problem hiding this comment.
I don't see cofx being used here, maybe I am missing something though
There was a problem hiding this comment.
wasn't used and found another unused one in the same file
| (-> (get-user-password! key-uid identity) | ||
| (.then #(security/hash-masked-password %)) | ||
| (.then #(save-user-password! key-uid %)) | ||
| (.then #(save-password-migration! key-uid)) | ||
| (.then #(callback)) |
There was a problem hiding this comment.
@cammellos @ilmotta I stumbled on the promesa package that has some really nice utilities for working with promises. It would be good for cases like this, where the snippet could be simplified to smth like:
(p/->> (get-user-password! key-uid identity)
(security/hash-masked-password)
(save-user-password! key-uid)
...)Do you think it would be a good addition to the project?
There was a problem hiding this comment.
@clauxx, I'm probably biased in favor of promesa because I extensively used it in the past. The library has a very solid API, so it's like one of those libs in Clojure where you add to the project and profit without maintenance burden.
OTOH, I used promesa in projects that heavily used promises everywhere (Node.js runtime instead of browser). Every code path had promises, so promesa paid off. In status-mobile we mostly have a few promises in re-frame effects (thankfully 😃). Therefore, our usages are very linear and simple, and I don't see promesa adding that much value.
I have also noticed promises code in status-mobile is quite convoluted and show some lack of knowledge of how promises work. Namespaces like react-native.cameraroll and react-native.permissions, among others show that to me because they confusingly mix callbacks with promises, probably an attempt to hide promises.
If we add promesa on top, I think it will be nice for some, but another layer on top of promises that some are not willing to learn.
In the end, I vote for adding it if you want in a separate PR, but better check with more devs.
There was a problem hiding this comment.
I saw the same issue in the react-native.keychain namespace, which is a bit annoying. Found myself pass identity as the callback in several places to make use of the resolved promise values. I'd say the value would be in having a consistent way of dealing with promises across the project and getting rid of the callbacks entirely, but this could be done without the library by just adding it in the guideline.
| (defn get-user-password! | ||
| [key-uid callback] | ||
| (keychain/get-credentials key-uid | ||
| #(callback (when % |
There was a problem hiding this comment.
I have the opinion, but also the impression from my experience with Clojure that the shorthand syntax #() should be used mostly for very simple functions, usually not spanning multiple lines. In this PR I see a lot of usages of this shorthand syntax in places where we (in status-mobile) wouldn't normally use. Hence, my feedback here is to ask you to be mindful of the shortcomings and lean on the side of clarity.
In the name of saving just a few characters we have a couple downsides:
- Obscures the argument with
%.%means nothing, so the developer is forced to read the surrounding context to guess what it is. - The developer can't be sure if the function is using any of the arguments without hunting for percent signs. This is often important. Contrast that with named arguments, where the linter will tell if the argument is used or not and it's immediately apparent to the developer.
- It makes debugging a little bit less enjoyable, because
#()doesn't allow multiple forms without wrapping in another macro likedoorlet. - It obscures the function signature. If the function has two or more arguments,
%1and%2can be difficult to understand, except on core functions where devs have memorized the arities, likereduce.
I have read some Clojure projects even outright banned #(), but I like it for simple one-liners.
There was a problem hiding this comment.
Thanks! Don't remember reading this in the guideline, but not a fan of #() either as it's tough on the eyes 🗡️ . Will keep an eye for this when refactoring in the future.
There was a problem hiding this comment.
It's not in the guidelines yet, but it would be useful if anyone is brave ;)
| key-uid | ||
| #(when-not % | ||
| (log/error | ||
| (str "Error while setting up keychain migration"))))) |
There was a problem hiding this comment.
I guess str here is a leftover when there were more arguments to concatenate, but it can be dropped now.
| (.then #(save-user-password! key-uid %)) | ||
| (.then #(save-password-migration! key-uid)) | ||
| (.then #(callback)) | ||
| (.catch #(log/error "Failed to migrate the keychain password for " key-uid |
There was a problem hiding this comment.
Right @cammellos, log/error can take pretty much anything, and the lib allows us to perform transformations with middlewares. The convention we try to follow in the repo is to pass a first argument string with a concise humanized message, and then the second argument a map giving enough context for the future dev to make sense of the error.
So here it could be:
(.catch (fn [error]
(log/error "Failed to migrate keychain password"
{:key-uid key-uid
:event :keychain/password-hash-migration
:error error})))The original code concatenating values won't be as clear. It will output something like Failed to migrate the keychain password for foo, but what is foo? Using a data structure the developer will know exactly what it is.
| (-> (get-user-password! key-uid identity) | ||
| (.then #(security/hash-masked-password %)) | ||
| (.then #(save-user-password! key-uid %)) | ||
| (.then #(save-password-migration! key-uid)) | ||
| (.then #(callback)) |
There was a problem hiding this comment.
@clauxx, I'm probably biased in favor of promesa because I extensively used it in the past. The library has a very solid API, so it's like one of those libs in Clojure where you add to the project and profit without maintenance burden.
OTOH, I used promesa in projects that heavily used promises everywhere (Node.js runtime instead of browser). Every code path had promises, so promesa paid off. In status-mobile we mostly have a few promises in re-frame effects (thankfully 😃). Therefore, our usages are very linear and simple, and I don't see promesa adding that much value.
I have also noticed promises code in status-mobile is quite convoluted and show some lack of knowledge of how promises work. Namespaces like react-native.cameraroll and react-native.permissions, among others show that to me because they confusingly mix callbacks with promises, probably an attempt to hide promises.
If we add promesa on top, I think it will be nice for some, but another layer on top of promises that some are not willing to learn.
In the end, I vote for adding it if you want in a separate PR, but better check with more devs.
| (if migrated? | ||
| (callback) | ||
| (-> (get-user-password! key-uid identity) | ||
| (.then #(security/hash-masked-password %)) |
There was a problem hiding this comment.
Can be just (.then security/hash-masked-password).
Also, it may be better to avoid calling callback if it's undefined or nil, so better wrap the call with a (when callback) check.
There was a problem hiding this comment.
Went for a default value for the callback when destructuring instead, so we don't check twice for the callback
| (-> (get-password-migration! key-uid identity) | ||
| (.then (fn [migrated?] | ||
| (if migrated? | ||
| (callback) |
There was a problem hiding this comment.
If calling callback throws an exception, it will be swallowed and ignored since no other code is piping a .catch on the return value of the re-frame effect.
You can move the .catch outside the inner function so it also catches errors from calling callback, and then improving the error message if it has been migrated or not.
You may also use try/catch around (callback) if you want to assume that part of the code is synchronous. I prefer not to mix and assume that, but it should be fine because we don't use the return value of this re-frame effect.
| @@ -154,7 +145,9 @@ | |||
| biometric-enabled? | |||
| (assoc :keychain/save-password-and-auth-method | |||
| {:key-uid key-uid | |||
| :masked-password masked-password | |||
| :masked-password (if syncing? | |||
There was a problem hiding this comment.
this reads a bit funny is the variable actually "masked" if syncing? is true?
If not it seems the variable naming should be adjusted some what here 👍
There was a problem hiding this comment.
So it's masked regardless of syncing?. The difference is that during syncing the password is already hashed (we don't have access to the plaintext password), so we need to hash it for the other cases.
| :accessibility-label :enable-biometrics-button | ||
| :icon-left :i/face-id | ||
| :customization-color profile-color | ||
| :theme theme |
There was a problem hiding this comment.
do we need to pass theme as a prop here? it should be normally got by the context mechanism within the component 👍
There was a problem hiding this comment.
yeah we don't, removed all mentions of the theme in the file.
| @@ -175,7 +190,7 @@ | |||
| cofx | |||
| {:db (assoc-in db [:profile/login :password] password)} | |||
| (navigation/init-root :progress) | |||
| (login)))) | |||
| (biometry-login)))) | |||
There was a problem hiding this comment.
is biometry appropriate here? should it not be biometrics-login ?
It kind of makes sense but never really heard of this domain referred to as "biometry"
There was a problem hiding this comment.
I was using them interchangeably till now, but they seem to be different 😮
src/utils/security/core.cljs
Outdated
| @@ -58,3 +59,8 @@ | |||
| and does not contain an rtlo character, which might mean that the url is spoofed" | |||
| [text] | |||
| (not (re-matches rtlo-link-regex text))) | |||
|
|
|||
| (def hash-masked-password | |||
There was a problem hiding this comment.
should we add simple tests for these utility methods?
cc @ilmotta
There was a problem hiding this comment.
You are asking the worst person :P I think pretty much all utility functions should have unit tests, simple or not. I refrain from commenting much in PRs about this, but I think this should be in our guidelines.
There was a problem hiding this comment.
Added tests for security/mask-data and security/hash-masked-password 👍
| (keychain/save-credentials | ||
| (password-migration-key-name key-uid) | ||
| key-uid | ||
| ;; NOTE: using the key-id as the password, but we don't really care about the |
There was a problem hiding this comment.
do you mean key-id or key-uid here? looks like a typo
| @@ -79,33 +78,88 @@ | |||
| (if can-save? | |||
| (keychain/get-credentials | |||
| (str key-uid "-auth") | |||
| #(callback (if % (oops/oget % "password") auth-method-none))) | |||
| (fn [value] | |||
| (callback (if value (oops/oget value "password") auth-method-none)))) | |||
There was a problem hiding this comment.
should "password" be in a private def ? 🤔
There was a problem hiding this comment.
So seems like oops considers const vars as "dynamic values" and throws warnings at compiled time, although it shouldn't for string constants. It would work with oops/oget+, but apparently it's slower .
There was a problem hiding this comment.
Not the same, but still related, I prefer to pass keywords to oget because they only need to be allocated once. It's not important really, just a nano optimization, but feels more idiomatic to me 🤷🏼
| [value] | ||
| (instance? MaskedData value)) | ||
|
|
||
| (schema/=> hash-masked-password |
There was a problem hiding this comment.
Lovely to see you are using malli already ❤️
@clauxx, may I suggest some improvements for readability?
Function schema errors already say if the error is in the input or output schema, so whatever custom schema we define, the error message doesn't need to hardcode if the it's the argument or return value. This allows us to create reusable schemas either for input/output values.
I think by extracting to a custom var schema, we can make the function schema signature clearer, which is one of the main selling points of spec'ing functions. If we leave the custom schema right in the schema definition as you did, this makes the code less readable.
So here's a suggestion:
(def ?masked-password
[:fn {:error/message "should be an instance of utils.security.core/MaskedData"}
masked-data-instance?])
(schema/=> hash-masked-password
[:=>
[:cat ?masked-password]
?masked-password])If schemas grow too much in the namespace, I think it's reasonable to create a namespace in the same directory named schema.cljs, but I like as you did since there's only one :)
There was a problem hiding this comment.
Another reason to prefer this style is that custom schemas can easily grow. For example, especially when using :fn schemas, malli won't know how to generate random values conforming to the schema, so we would need a :schema/gen generator in the definition, thus making the inlined schema less readable still. So by extracting custom schemas to vars we promote more readable function schemas now and in the future as they evolve.
There was a problem hiding this comment.
yep, it makes perfect sense! I like this way much better. Thanks!!
pavloburykh
force-pushed
the
17928-sync-biometry-no-password
branch
from
3e9ce16 to
adbc3e8
Compare
pavloburykh
force-pushed
the
17928-sync-biometry-no-password
branch
from
10b0bd7 to
326db65
Compare
62% of end-end tests have passedFailed tests (15)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (30)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@clauxx @cammellos got one question: could current changes somehow affect data delivery reliability? For some reason lot of e2e are failing because CR has not been delivered or community status remains Pending instead of switching to Joined and so on. This is suspicious, cause I don't observe such failures in other PRs that do not contain any go changes. @clauxx let's hold on with merging it, I want to make sure that described problems 100% not related to the PR changes. I will compare e2e results tomorrow with nightly/other prs. So far I cannot reproduce manually those issues caught by e2e. But I have made lot of re-runs and tests keep failing. |
|
@clauxx today e2e are failing in other PRs too. So these failures are not PR related. We can merge it. |
* feat: added migration for the keychain hashed password * feat: added sync biometry without password entry * fix: biometry typo from develop * ref: moved migration side-effects outside the event * ref: some renaming for keychain migration * ref: addressed @cammellos' review comments * ref: removed unnecessary anon fn * fix: addressed @ilmotta's review comments * ref: removed theme from enable-biometrics * ref: addressed J-Son89's review comments * test: added tests for mask-data and hash-masked-password * test: added schema to hash-masked-password and fixed test * fix: forgot the threading * ref: improved the masked data schema * fix: no biometry error when canceled by user * fix: biometry error wasn't propagated during login * fix: alert dismiss button not passed properly * fix: show biometrics NOT_ENROLLED error only once * lint: removed unused require
fixes #17928
Summary
Currently, when the user syncs device A with device B (mobile), the user is prompted to input the device A password to enable biometry. This was needed because the device A password comes hashed during syncing, while for biometry authentication we need the password in plaintext for the keychain.
As agreed with @cammellos and @Samyoul, we should be moving away from storing the plaintext password in the keychain in favor of storing it already hashed (we only need it hashed anyway). As such, this PR:
Testing notes
To test the migration:
If it fails to log in during step 3, then there's something wrong with the migration.
Platforms
Areas that maybe impacted
Functional
Steps to test
When enabling biometry, the password auth should not appear and the user should be able to log it again with biometry after the app was closed.
status: ready